Handling of empty string to datetime conversion #2891

srfrog · 2019-01-12T09:13:43Z

This PR fixes an issue when converting empty string to datetime value.

When altering a schema from string to datetime that has empty strings, Dgraph fails because it doesn't know how to convert empty string values to datetime.
Int and float zero values are converted to zero-time value, and zero when converted back.
Conversions from string to datetime use zero time value.
New tests added for Convert cases.

New behavior

int: convert 0 to datetime -> zero time
float: convert 0 to datetime -> zero time
string: convert "" to datetime -> zero time
binary: convert '' to datetime -> zero time

datetime: convert zero time to int -> 0
datetime: convert zero time to float -> 0
datetime: convert zero-time to string -> ""
datetime: convert zero-time to binary -> ''

Closes #2310

This change is

linter fixes.

manishrjain

int: convert 0 to datetime -> epoch
float: convert 0 to datetime -> epoch
string: convert "" to datetime -> zero time

I'd argue that they should all be zero time. Zero int, Zero float and empty string are default values for each of these types, and they should default to empty time{} struct, which is Zero time.

Looks mostly good. Except this change.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @srfrog and @manishrjain)

types/conversion.go, line 223 at r1 (raw file):

			case BinaryID:
				if vc {
					*res = []byte{1}

How about:

*res = []byte{0}
if vc { *res = []byte{1} }

Avoids the break statements. Here and below.

types/conversion_test.go, line 526 at r1 (raw file):

		// 0
		{in: Val{Tid: IntID, Value: []byte{0, 0, 0, 0, 0, 0, 0, 0}},
			out: Val{Tid: DateTimeID, Value: time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)}},

This should be Zero time.

types/conversion_test.go, line 548 at r1 (raw file):

		// 0
		{in: Val{Tid: FloatID, Value: []byte{0, 0, 0, 0, 0, 0, 0, 0}},
			out: Val{Tid: DateTimeID, Value: time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)}},

Should be Zero time.

code formatting changes.

added more tests and fixed format.

srfrog

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

types/conversion.go, line 223 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

How about:
*res = []byte{0}
if vc { *res = []byte{1} }
Avoids the break statements. Here and below.

Done.

types/conversion_test.go, line 526 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

This should be Zero time.

Done.

types/conversion_test.go, line 548 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Should be Zero time.

Done.

manishrjain

This PR makes tests a lot harder to read. I'm not sure why was there a need to change them?

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @srfrog)

types/conversion_test.go, line 526 at r2 (raw file):

	}{
		// time.Date(2009, time.November, 10, 23, 0, 0, 0, time.UTC)
		{in: Val{Tid: DateTimeID, Value: []byte{1, 0, 0, 0, 14, 194, 139, 231, 112, 0, 0, 0, 0, 255, 255}},

The tests before were a lot more readable. Byte arrays are impossible to understand. Also, moving time.Date to comment isn't great.

srfrog

I'll add some spaces. Most of these tests are new or rewritten, the file was mostly outdated and commented out.

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @srfrog)

srfrog

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

types/conversion_test.go, line 526 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The tests before were a lot more readable. Byte arrays are impossible to understand. Also, moving time.Date to comment isn't great.

Done.

manishrjain

One comment. See if you'd like to address it before submitting.

Reviewed 1 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @srfrog)

types/conversion.go, line 75 at r3 (raw file):

			case DateTimeID:
				var t time.Time
				if !bytes.Equal(data, []byte("")) {

if len(data) > 0?

types/conversion.go, line 285 at r3 (raw file):

				} else {
					secs := t.Unix()
					if secs > math.MaxInt64 || secs < math.MinInt64 {

Not your code, but would this condition ever be true? Think never.

types/conversion_test.go, line 398 at r3 (raw file):

		{
			in:  float64(3.0),
			out: true,

I think you should be able to line in a single line, instead of splitting these over 4 lines each.

Would improve the readability considerably. Here and elsewhere.

srfrog

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

types/conversion.go, line 75 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

if len(data) > 0?

No, data is a slice so nil would be the same as empty slice.

Ex: https://play.golang.camp/p/gQKQuP4fjIw

types/conversion.go, line 285 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not your code, but would this condition ever be true? Think never.

Yes, the first test will fail on "2147483647-12-31 13:00:01.147483647 +0000 UTC" but then we would be dust and it wont matter. The other case will fail if we go back in time before epoch and adjust our clock :)

types/conversion_test.go, line 398 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I think you should be able to line in a single line, instead of splitting these over 4 lines each.

Would improve the readability considerably. Here and elsewhere.

Done.

…ue-2310_empty_datetime_parsing

srfrog

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @manishrjain)

types/conversion.go, line 285 at r3 (raw file):

Previously, srfrog (Gus) wrote…

Yes, the first test will fail on "2147483647-12-31 13:00:01.147483647 +0000 UTC" but then we would be dust and it wont matter. The other case will fail if we go back in time before epoch and adjust our clock :)

Done.

* commented unused func indexCellsForCap (linter). * let conversion of empty string to datetime work for zero time value. linter fixes. * added Converter tests * linter fixes * removed unused func createDate and other linter fixes * convert zero-time value to empty/zero values of other types. code formatting changes. * added tests for 0 int/float to convert to datatime zero-time value. added more tests and fixed format. * refactored tests for better legibility and support * removed unlikely test on time value * reformatted tests * minor cleanups * added binary, bool and datetime tests * added bool to string test * minor cleanups

srfrog added 5 commits January 12, 2019 01:54

commented unused func indexCellsForCap (linter).

2f75aa0

let conversion of empty string to datetime work for zero time value.

15d7892

linter fixes.

added Converter tests

23b0c58

linter fixes

4b68576

removed unused func createDate and other linter fixes

666829c

srfrog added the in progress label Jan 12, 2019

srfrog requested a review from manishrjain January 14, 2019 18:57

manishrjain suggested changes Jan 15, 2019

View reviewed changes

srfrog added 2 commits January 14, 2019 20:45

convert zero-time value to empty/zero values of other types.

ecfc32e

code formatting changes.

added tests for 0 int/float to convert to datatime zero-time value.

7db4a45

added more tests and fixed format.

srfrog removed the in progress label Jan 15, 2019

srfrog commented Jan 15, 2019

View reviewed changes

srfrog added the breaking_change label Jan 16, 2019

manishrjain suggested changes Jan 16, 2019

View reviewed changes

srfrog commented Jan 16, 2019

View reviewed changes

refactored tests for better legibility and support

c734fa3

srfrog commented Jan 17, 2019

View reviewed changes

manishrjain approved these changes Jan 18, 2019

View reviewed changes

srfrog added 2 commits January 18, 2019 11:28

removed unlikely test on time value

3b022fa

reformatted tests

8130f97

srfrog commented Jan 18, 2019

View reviewed changes

srfrog added 3 commits January 21, 2019 21:40

Merge branch 'master' of github.com:/dgraph-io/dgraph into srfrog/iss…

544a889

…ue-2310_empty_datetime_parsing

minor cleanups

61e1bf6

added binary, bool and datetime tests

3089b91

srfrog commented Jan 22, 2019

View reviewed changes

srfrog added 2 commits January 21, 2019 22:30

added bool to string test

2b0b57b

minor cleanups

ef6aaf6

srfrog merged commit f6a2c04 into master Jan 22, 2019

srfrog deleted the srfrog/issue-2310_empty_datetime_parsing branch January 22, 2019 05:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling of empty string to datetime conversion #2891

Handling of empty string to datetime conversion #2891

srfrog commented Jan 12, 2019 •

edited

Loading

manishrjain left a comment

srfrog left a comment

manishrjain left a comment

srfrog left a comment

srfrog left a comment

manishrjain left a comment

srfrog left a comment

srfrog left a comment

Handling of empty string to datetime conversion #2891

Handling of empty string to datetime conversion #2891

Conversation

srfrog commented Jan 12, 2019 • edited Loading

manishrjain left a comment

Choose a reason for hiding this comment

srfrog left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

srfrog left a comment

Choose a reason for hiding this comment

srfrog left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

srfrog left a comment

Choose a reason for hiding this comment

srfrog left a comment

Choose a reason for hiding this comment

srfrog commented Jan 12, 2019 •

edited

Loading